Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enterprise Search] [Search application] List indices fetch indices stats from ES directly #153696

Merged
merged 13 commits into from
Mar 30, 2023

Conversation

saarikabhasi
Copy link
Member

@saarikabhasi saarikabhasi commented Mar 24, 2023

Summary

This PR adds a lib file which fetches the indices stats of an search application from Elasticsearch directly

Screenshot

List page indices

List page indices

Overview page indices

Overview page indices

Checklist

@saarikabhasi saarikabhasi marked this pull request as ready for review March 24, 2023 19:12
@saarikabhasi saarikabhasi requested a review from a team March 24, 2023 19:12
@@ -20,11 +20,14 @@ export interface EnterpriseSearchEngine {
}

export interface EnterpriseSearchEngineDetails {
indices: EnterpriseSearchEngineIndex[];
indices: string[];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indices are of type string[] from ES but the hydrated indices is of type EnterpriseSearchEngineIndex[]. Hence added EnterpriseSearchEngineDetailsResponse

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type was only needed because the get engine & list engines results differed. since this is returning indices: string[]; now we can just use EnterpriseSearchEngine as the types are the same.

Copy link
Member

@kderusso kderusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and clean! One nitpick, could you please update the PR title to be more descriptive of the change? Thank you!

@saarikabhasi saarikabhasi changed the title Initial commit [Enterprise Search] [Search application] List indices fetch indices stats from ES directly Mar 24, 2023
@saarikabhasi saarikabhasi added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.8.0 Team:EnterpriseSearch labels Mar 24, 2023
@saarikabhasi saarikabhasi requested a review from a team March 24, 2023 19:20
Copy link
Member

@sphilipse sphilipse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job! Just one spelling nitpick

metric: ['docs'],
});
const indicesWithStats = indicesNames.map((indexName: string) => {
const indiceStats = indicesStats[indexName];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick(non-blocking): indiceStats should be indicesStats (or maybe indexStats).

@saarikabhasi saarikabhasi force-pushed the search-app-update-list-page branch from 8d3baf7 to 6916094 Compare March 27, 2023 20:20
@saarikabhasi
Copy link
Member Author

@elasticmachine merge upstream

updated_at_millis: number;
name: string;
}

export interface EnterpriseSearchEngineDetailsResponse
Copy link
Contributor

@TattdCodeMonkey TattdCodeMonkey Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

introducing this type doesn't make sense to me. I think we should update EnterpriseSearchEngineDetails with indices: EnterpriseSearchEngineIndex[]; instead of introducing another interface here.

@saarikabhasi saarikabhasi force-pushed the search-app-update-list-page branch from d30f363 to 1e3a2f3 Compare March 27, 2023 23:33
@saarikabhasi saarikabhasi force-pushed the search-app-update-list-page branch from 1e3a2f3 to 705935b Compare March 27, 2023 23:35
@saarikabhasi
Copy link
Member Author

@elasticmachine merge upstream

@@ -14,15 +14,9 @@ export interface EnterpriseSearchEnginesResponse {
}

export interface EnterpriseSearchEngine {
Copy link
Contributor

@TattdCodeMonkey TattdCodeMonkey Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we hydrating indices with the full index data for the list request?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List all engines and GET a engine request returns list indices names in String[]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated_at_millis: number;
}

export interface EnterpriseSearchEngineDetails {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll still need this type to be different from EnterpriseSearchEngine since the list and get responses return different data. List results has indices: string[] and should use EnterpriseSearchEngine and the get for a single search application will need to return a EnterpriseSearchEngineDetails which has the indices hydrated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it make sense.

General question:

If we define indices: EnterpriseSearchEngineIndex[] in EnterpriseSearchEngineDetails but GET a single search application response indices type is String[] would that be any issue in general ?

On side note we do update the hydrated indices in here

@saarikabhasi saarikabhasi force-pushed the search-app-update-list-page branch from 7eb14bf to ea21c98 Compare March 28, 2023 15:24
@saarikabhasi
Copy link
Member Author

`@elasticmachine merge upstream

@saarikabhasi
Copy link
Member Author

@elasticmachine merge upstream

@saarikabhasi saarikabhasi force-pushed the search-app-update-list-page branch from 5ba36eb to c383c5f Compare March 29, 2023 14:07
@saarikabhasi saarikabhasi force-pushed the search-app-update-list-page branch from 13f58ae to 6100bc9 Compare March 29, 2023 15:39
@saarikabhasi saarikabhasi enabled auto-merge (squash) March 30, 2023 13:28
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 432 435 +3

Total ESLint disabled count

id before after diff
securitySolution 512 515 +3

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@saarikabhasi saarikabhasi merged commit 327dd49 into elastic:main Mar 30, 2023
@saarikabhasi saarikabhasi deleted the search-app-update-list-page branch March 30, 2023 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:EnterpriseSearch v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants